Skip to content

Comments

feat: Implement Scrapy HTTP cache backend #403

Merged
vdusek merged 18 commits intoapify:masterfrom
honzajavorek:honzajavorek/cache
Mar 18, 2025
Merged

feat: Implement Scrapy HTTP cache backend #403
vdusek merged 18 commits intoapify:masterfrom
honzajavorek:honzajavorek/cache

Conversation

@honzajavorek
Copy link
Contributor

@honzajavorek honzajavorek commented Feb 14, 2025 •
edited by vdusek
Loading

I successfully use this code in my project. In 381c044 I explicitly specify licensing. I didn't add docs or anything else (yet), just made sure the code passes linters and a type check.

Relates: apify/actor-templates#303

@honzajavorek honzajavorek changed the title Implement Scrapy HTTP cache backend feat: Implement Scrapy HTTP cache backend Feb 14, 2025
@honzajavorek honzajavorek temporarily deployed to fork-pr-integration-tests February 14, 2025 17:01 — with GitHub Actions Inactive
This was referenced Feb 14, 2025
Closed
Closed
vdusek
vdusek requested changes Feb 18, 2025
View reviewed changes
Copy link
Contributor

@vdusek vdusek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To make this work by default, also adjust the apify.scrapy.utils.apply_apify_settings. If this allows us to handle platform migration, we should turn caching by default. Also please add a reasonable comments there.

@vdusek vdusek added t-tooling Issues with this label are in the ownership of the tooling team. enhancement New feature or request. labels Feb 18, 2025
@vdusek vdusek added this to the 108th sprint - Tooling team milestone Feb 18, 2025
@honzajavorek
Copy link
Contributor Author

honzajavorek commented Feb 18, 2025

Thanks for the review! I considered this to be a kick off which will definitely get a review with changes requested. If #404 lets me, I'll work on addressing the comments in the upcoming days.

@honzajavorek
Copy link
Contributor Author

honzajavorek commented Feb 18, 2025

I bet I've seen comments about adding this to Apify settings by default, but I can't see them now. I think it's a good idea to set the cache storage by default, but I wouldn't turn on caching by default, because it's application-specific how caching should work exactly (e.g. expiration time). We should just document somewhere that this is a preferred solution to handling the forced restarts, if they happen.

@vdusek
Copy link
Contributor

vdusek commented Feb 19, 2025

I bet I've seen comments about adding this to Apify settings by default, but I can't see them now. I think it's a good idea to set the cache storage by default, but I wouldn't turn on caching by default, because it's application-specific how caching should work exactly (e.g. expiration time). We should just document somewhere that this is a preferred solution to handling the forced restarts, if they happen.

Agreed, thanks 🙂.

@honzajavorek
Copy link
Contributor Author

honzajavorek commented Mar 10, 2025

Just for the record, the comment I've seen didn't get lost, it's the final comment of the review 😄

@honzajavorek
Copy link
Contributor Author

honzajavorek commented Mar 10, 2025

I think I'm done for now!

@honzajavorek honzajavorek requested a review from vdusek March 10, 2025 14:20
vdusek
vdusek reviewed Mar 11, 2025
View reviewed changes
@vdusek vdusek mentioned this pull request Mar 13, 2025
Open
@honzajavorek
feat(scrapy): add Scrapy cache using Apify KV store
30c1e97 
This code has been originally developed for the
https://github.com/juniorguru/plucker/ project, which is licensed under
AGPL-3.0-only. I am the sole author of that code and hereby I grant
the https://github.com/apify/apify-sdk-python/ project the right to use
it under an Apache-2.0 license, without the copyleft taking effect.
My intention is to contribute this code to the upstream, remove
it from my project, and only import the component from the apify
package as a dependency, as I believe this component could be useful
to other users of the apify package.
@honzajavorek
style: format code
96f1142
@honzajavorek
fix: make linter happy
40076f3
@honzajavorek
fix: return back nested syntax
eb5b73a 
This error was introduced when I tried to make all
the linters happy, uh.
@honzajavorek
feat: introduce the extensions package
ece55b1
@honzajavorek
refactor: make linter happy
468d3d2
@honzajavorek
fix: don't use public properties
48ee026
@honzajavorek
fix: rename module to httpcache and fix tests location
f6701db
@honzajavorek
docs: improve docstring of ApifyCacheStorage, describe usage
21ce5fc
@honzajavorek
refactor: rename kv to kvs per convention
c19a93e
@honzajavorek
feat: set HTTPCACHE_STORAGE in apply_apify_settings, document usage
d8adf62
@honzajavorek
docs: improve stylistics
1b02e45
@honzajavorek
docs: document workaround for apify/actor-templates#303
b6969ba
@honzajavorek honzajavorek requested a review from vdusek March 14, 2025 11:05
vdusek

This comment was marked as resolved.

Sign in to view

vdusek
vdusek reviewed Mar 14, 2025
View reviewed changes
@honzajavorek honzajavorek temporarily deployed to fork-pr-integration-tests March 14, 2025 11:29 — with GitHub Actions Inactive
@vdusek vdusek temporarily deployed to fork-pr-integration-tests March 14, 2025 11:52 — with GitHub Actions Inactive
vdusek
vdusek reviewed Mar 18, 2025
View reviewed changes
@honzajavorek honzajavorek temporarily deployed to fork-pr-integration-tests March 18, 2025 13:13 — with GitHub Actions Inactive
@honzajavorek honzajavorek temporarily deployed to fork-pr-integration-tests March 18, 2025 13:56 — with GitHub Actions Inactive
@honzajavorek honzajavorek temporarily deployed to fork-pr-integration-tests March 18, 2025 13:59 — with GitHub Actions Inactive
@honzajavorek honzajavorek temporarily deployed to fork-pr-integration-tests March 18, 2025 13:59 — with GitHub Actions Inactive
vdusek
vdusek approved these changes Mar 18, 2025
View reviewed changes
Copy link
Contributor

@vdusek vdusek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks

@vdusek vdusek merged commit 137e3c8 into apify:master Mar 18, 2025
25 of 27 checks passed
@vdusek vdusek mentioned this pull request Mar 18, 2025
Closed
@honzajavorek
Copy link
Contributor Author

honzajavorek commented Mar 18, 2025

cat-highfive

@honzajavorek honzajavorek deleted the honzajavorek/cache branch March 18, 2025 22:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vdusek vdusek vdusek approved these changes

Assignees

No one assigned

Labels

enhancement New feature or request. t-tooling Issues with this label are in the ownership of the tooling team.

Projects

None yet

Milestone

108th sprint - Tooling team

Development

Successfully merging this pull request may close these issues.

2 participants

@honzajavorek @vdusek